chore(scripts): add Windows fixes verification script #1108
Conversation
Add a script to verify Windows-specific PRs are working correctly: - PR #1: Marketplace initialization check - PR #2: Error surfacing code presence check - PR #3: Windows path normalization test - PR #4: Token hot-reload IPC channel check Usage: node scripts/test-windows-fixes.js [--all] [--pr N] Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new Node.js verification script ( Changes
Sequence Diagram(s)(omitted — changes are a single verification script; no multi-component flow requiring diagram) Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @youngmrz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a crucial new utility script designed to enhance the quality assurance process for Windows-specific functionalities within the Auto-Claude project. By automating checks for common Windows-related issues, the script aims to ensure the stability and correctness of fixes, providing a reliable way to prevent regressions and validate platform-specific behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a helpful verification script for Windows-specific fixes. The script is well-structured and provides clear, color-coded output. My review focuses on improving the script's robustness, correctness, and maintainability. I've identified a few issues, including a bug in one of the test functions, fragile command-line argument parsing, and opportunities to make shell command execution and path resolution more reliable across different platforms. The suggested changes will make the script more dependable for its intended purpose.
| if (content.includes(check.search)) { | ||
| success(`${check.name}: Found "${check.search}"`); | ||
| } else { | ||
| info(`${check.name}: "${check.search}" not found (PR #4 may not be merged yet)`); | ||
| } |
There was a problem hiding this comment.
The logic for this check is incorrect. When content.includes(check.search) is false, an info message is logged, but allPassed is not updated to false. This causes the test to be reported as passed even when the check fails. To ensure the test fails correctly, you should set allPassed = false and consider logging an error instead of info.
if (content.includes(check.search)) {
success(`${check.name}: Found "${check.search}"`);
} else {
error(`${check.name}: "${check.search}" not found (PR #4 may not be merged yet)`);
allPassed = false;
}
scripts/test-windows-fixes.js
Outdated
| const args = process.argv.slice(2); | ||
| const runAll = args.includes('--all'); | ||
| const prArg = args.find(a => a.startsWith('--pr')); | ||
| const specificPR = prArg ? parseInt(prArg.split('=')[1] || args[args.indexOf('--pr') + 1]) : null; |
There was a problem hiding this comment.
The argument parsing for --pr is a bit fragile. If a user runs the script with --pr but without a number (e.g., node scripts/test-windows-fixes.js --pr), specificPR becomes NaN. The script then silently falls back to running all tests instead of reporting an error, which can be confusing. You should add validation to handle this case and provide a clear error message.
For example:
if (prArg && isNaN(specificPR)) {
error('Invalid or missing PR number for --pr argument.');
process.exit(1);
}Also, it's a good practice to always provide the radix parameter to parseInt to avoid unexpected behavior.
const specificPR = prArg ? parseInt(prArg.split('=')[1] || args[args.indexOf('--pr') + 1], 10) : null;
scripts/test-windows-fixes.js
Outdated
|
|
||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const { execSync, spawn } = require('child_process'); |
There was a problem hiding this comment.
The spawn function is imported from child_process but is never used in the script. Removing unused imports helps keep the code clean and maintainable.
| const { execSync, spawn } = require('child_process'); | |
| const { execSync } = require('child_process'); |
scripts/test-windows-fixes.js
Outdated
| function testPR1_MarketplaceInit() { | ||
| log('\n--- PR #1: Windows Marketplace Fix ---', 'cyan'); | ||
|
|
||
| const appDataDir = process.env.APPDATA || path.join(process.env.HOME || '', '.config'); |
There was a problem hiding this comment.
The logic for determining the application data directory is not fully cross-platform. It handles Windows and Linux-like systems but doesn't correctly identify the path on macOS (which should be ~/Library/Application Support).
To make this more robust, you could use os.homedir() and check process.platform. You'll need to add const os = require('os'); at the top of the file.
A more robust implementation could look like this:
function getAppDataDir() {
const os = require('os');
const homedir = os.homedir();
switch (process.platform) {
case 'win32':
return process.env.APPDATA || path.join(homedir, 'AppData', 'Roaming');
case 'darwin':
return path.join(homedir, 'Library', 'Application Support');
default: // linux, etc.
return process.env.XDG_CONFIG_HOME || path.join(homedir, '.config');
}
}
const appDataDir = getAppDataDir();Using os.homedir() instead of process.env.HOME is also more reliable.
scripts/test-windows-fixes.js
Outdated
| execSync('cd apps/frontend && npx tsc --noEmit', { | ||
| cwd: path.join(__dirname, '..'), | ||
| stdio: 'pipe', | ||
| timeout: 120000, | ||
| }); |
There was a problem hiding this comment.
Chaining commands with && inside execSync can be less robust than using the cwd option to specify the working directory. It's better to set the cwd directly to the apps/frontend directory and execute the command there.
execSync('npx tsc --noEmit', {
cwd: path.join(__dirname, '..', 'apps', 'frontend'),
stdio: 'pipe',
timeout: 120000,
});
scripts/test-windows-fixes.js
Outdated
| execSync('npm test -- --run', { | ||
| cwd: path.join(__dirname, '..'), | ||
| stdio: 'inherit', | ||
| timeout: 300000, | ||
| }); |
There was a problem hiding this comment.
Similar to the TypeScript check, it's more robust to run this command directly in the apps/frontend directory by setting the cwd option. Also, the --run argument passed to npm test seems redundant since the script vitest run already implies running the tests once. You can simplify this command.
execSync('npm test', {
cwd: path.join(__dirname, '..', 'apps', 'frontend'),
stdio: 'inherit',
timeout: 300000,
});![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@scripts/test-windows-fixes.js`:
- Line 15: Remove the unused destructured import "spawn" from the require call
so only execSync is imported; change the line containing "const { execSync,
spawn } = require('child_process');" to import only execSync (e.g., "const {
execSync } = require('child_process');") and ensure no other references to spawn
exist in the file.
- Around line 193-207: The execSync invocation currently prefixes the command
with "cd apps/frontend &&" which is redundant because cwd is already set to the
repo root; update the execSync call to run just "npx tsc --noEmit" and change
its cwd option to point directly at the frontend package (e.g. use
path.join(__dirname, '..', 'apps', 'frontend')) so the TypeScript check runs in
the correct directory without shell cd. Locate the execSync call in the try
block and adjust the command string and cwd accordingly.
- Around line 245-246: The current prArg detection uses a loose prefix match
(a.startsWith('--pr')) which can match flags like --proxy; update the logic that
sets prArg and specificPR to only accept either an exact '--pr' token or a
'--pr=<num>' form: find an arg where arg === '--pr' or arg.startsWith('--pr='),
and then parse the value either from the substring after '=' (for '--pr=123') or
from the next args element when the token is exactly '--pr'; adjust the code
around prArg and specificPR to use this stricter matching and robustly handle
missing/invalid numbers.
- Around line 92-105: Add an explicit branch to handle the case where
errorPatterns exists but stderrCollected does not: check if hasErrorPatterns &&
!hasStderrCollected and in that branch call warn('errorPatterns present but
stderrCollected missing') (or similar descriptive message) and return { passed:
false }; keep the existing success branch for both true and the current else for
both false (skipped) so all four combinations are handled; refer to the existing
variables hasStderrCollected, hasErrorPatterns and the warning/success/info
helpers when implementing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
scripts/test-windows-fixes.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Seer Code Review
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
scripts/test-windows-fixes.js (4)
17-33: LGTM!Clean implementation of color-coded logging helpers with appropriate ANSI escape codes.
39-74: LGTM!Good defensive coding with proper handling of missing directories/files and JSON validation. The fallback logic for
APPDATAis reasonable for cross-platform testing.
213-228: LGTM!Reasonable timeout and proper use of
stdio: 'inherit'to stream test output in real-time.
273-303: LGTM!Clear summary output with proper exit code handling for CI integration. The distinction between passed, failed, and skipped states is well implemented.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Address HIGH and MEDIUM severity feedback from Gemini and CodeRabbit: - Remove unused spawn import from child_process - Add os module and fix cross-platform app data directory resolution (properly handles Windows, macOS, and Linux) - Fix logic gap in testPR2 where errorPatterns without stderrCollected was not handled as a failure case - Fix bug in testPR4 where missing search strings didn't fail the test (now properly sets allPassed = false) - Use cwd option directly instead of 'cd apps/frontend &&' for execSync - Improve --pr argument parsing with proper validation and clear error message when number is missing or invalid Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: youngmrz <[email protected]>
|
@gemini-code-assist review The issues flagged in the previous review have been addressed in commit ba3d36d. Please re-review. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
|
Hello @youngmrz, Thank you for requesting a re-review and for addressing the previous feedback. I've reviewed the changes in commit Overall, the script is well-structured, readable, and effectively addresses the need for Windows-specific fix verification. The use of dedicated functions for each PR test, clear logging with color-coding, and robust argument parsing make it a valuable addition to the project's testing utilities. Docstring CoverageRegarding the docstring coverage warning previously flagged by CodeRabbit (and confirmed by my analysis), the new script currently has 7 out of 12 functions documented with JSDoc comments, resulting in approximately 58.33% coverage. If the project's required threshold for docstring coverage is 80%, you might consider adding JSDoc comments to the helper logging functions ( For example, you could add a simple JSDoc to /**
* Logs a message to the console with optional color.
* @param {string} msg - The message to log.
* @param {string} [color='reset'] - The color to use for the message (e.g., 'green', 'red').
*/
function log(msg, color = 'reset') {
console.log(`${COLORS[color]}${msg}${COLORS.reset}`);
}This is a Let me know if you have any further questions! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/test-windows-fixes.js`:
- Around line 281-290: The switch default for specificPR only logs via
error(...) and leaves results unchanged, causing a misleading success exit; fix
by making the default either push a failing entry into results (e.g., add a
results.push(...) with a descriptive name like `PR #${specificPR}` and a
failed/error status so failedCount increments) or call process.exit(1)
immediately after error(...) to return a non‑zero exit code; update the switch
default handling around specificPR and results (and any downstream failedCount
computation) so invalid PR numbers produce a non-zero exit and show a failing
summary.
♻️ Duplicate comments (1)
scripts/test-windows-fixes.js (1)
124-160: Test validates Node.js built-in behavior, not PR#3code artifacts.This function tests
path.normalize()which is Node.js's built-in behavior, not any custom Windows path normalization code that PR#3may have introduced. UnliketestPR2andtestPR4which check for specific code patterns in source files, this test doesn't verify any PR#3implementation.Consider either:
- Adding checks for specific PR
#3code artifacts (e.g., searching for path-handling utilities in the codebase), or- Documenting this as a "platform capability smoke test" rather than a PR verification.
Additionally,
path.normalize()doesn't throw for string inputs, making the try-catch (lines 138-145) unnecessary:Simplify the loop
for (const test of testCases) { - try { - // Use path.normalize which should work on all platforms - const normalized = path.normalize(test.input); - success(`${test.desc}: "${test.input}" → "${normalized}"`); - } catch (e) { - error(`${test.desc}: Failed to normalize "${test.input}": ${e.message}`); - allPassed = false; - } + const normalized = path.normalize(test.input); + success(`${test.desc}: "${test.input}" → "${normalized}"`); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
scripts/test-windows-fixes.js
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
🔇 Additional comments (8)
scripts/test-windows-fixes.js (8)
1-16: LGTM!The shebang, documentation header, and imports are well-structured. The unused
spawnimport has been removed as noted in past reviews.
18-34: LGTM!Clean, idiomatic logging helpers with consistent ANSI color support.
40-83: LGTM!Cross-platform app data directory resolution now correctly handles Windows, macOS, and Linux using
os.homedir()as previously recommended. The validation logic properly handles all file states.
89-118: LGTM!The logic gap from the previous review has been addressed. All four combinations of
stderrCollectedanderrorPatternspresence are now explicitly handled with appropriate pass/fail/skip results.
166-198: LGTM!The bug from the previous review has been fixed. Missing search strings now correctly fail the test by setting
allPassed = falseand logging witherror()instead ofinfo().
203-221: LGTM!The
cwdoption is now correctly used to run TypeScript compilation directly in the frontend directory, as previously recommended.
226-242: LGTM!Correctly uses
cwdoption for the frontend directory with appropriate timeout for test execution.
303-334: LGTM!Summary aggregation and exit code logic are correct. The hint about
--allis helpful for users.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
scripts/test-windows-fixes.js
Outdated
| case 2: results.push({ name: 'PR #2', ...testPR2_ErrorSurfacing() }); break; | ||
| case 3: results.push({ name: 'PR #3', ...testPR3_WindowsPathFix() }); break; | ||
| case 4: results.push({ name: 'PR #4', ...testPR4_TokenHotReload() }); break; | ||
| default: error(`Unknown PR number: ${specificPR}`); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
When an invalid PR number is passed to --pr, the script now exits with code 1 instead of silently continuing with 0 passed/0 failed. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: youngmrz <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| function testPR2_ErrorSurfacing() { | ||
| log('\n--- PR #2: Error Surfacing Fix ---', 'cyan'); | ||
|
|
||
| const agentProcessPath = path.join(__dirname, '../apps/frontend/src/main/agent/agent-process.ts'); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
🔴 Blocked - 1 CI check(s) failing. Fix CI before merge.
Blocked: 1 CI check(s) failing. Fix CI before merge.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Medium | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Branch Out of Date: PR branch is behind the base branch and needs to be updated
- CI Failed: Require Re-review on Push
Findings Summary
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (1 selected of 1 total)
🔵 [59b7c438476c] [LOW] PR #3 test uses Node.js built-in path.normalize only
📁 scripts/test-windows-fixes.js:148
The testPR3_WindowsPathFix() function tests Node.js built-in path.normalize() rather than verifying actual project-specific Windows path handling utilities. While this serves as a basic smoke test, it does not validate that any custom path normalization code in the project works correctly. Consider enhancing to test actual project utilities if they exist.
Suggested fix:
Add tests that verify actual project-specific path handling utilities if such utilities exist, rather than only testing Node.js built-ins
This review was generated by Auto Claude.
youngmrz
force-pushed
the
util/windows-test-script
branch
2 times, most recently
from
64bec0e to
e10342d
Compare
scripts/test-windows-fixes.js
Outdated
|
|
||
| const content = fs.readFileSync(check.path, 'utf8'); | ||
| if (content.includes(check.search)) { | ||
| success(`${check.name}: Found "${check.search}"`); | ||
| } else { | ||
| error(`${check.name}: "${check.search}" not found`); | ||
| allPassed = false; | ||
| } | ||
| } | ||
|
|
||
| return { passed: allPassed }; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add a script to verify Windows-specific PRs are working correctly: - PR #1: Marketplace initialization check - PR #2: Error surfacing code presence check - PR #3: Windows path normalization test - PR #4: Token hot-reload IPC channel check Usage: node scripts/test-windows-fixes.js [--all] [--pr N] Co-Authored-By: Claude Opus 4.5 <[email protected]>
Address HIGH and MEDIUM severity feedback from Gemini and CodeRabbit: - Remove unused spawn import from child_process - Add os module and fix cross-platform app data directory resolution (properly handles Windows, macOS, and Linux) - Fix logic gap in testPR2 where errorPatterns without stderrCollected was not handled as a failure case - Fix bug in testPR4 where missing search strings didn't fail the test (now properly sets allPassed = false) - Use cwd option directly instead of 'cd apps/frontend &&' for execSync - Improve --pr argument parsing with proper validation and clear error message when number is missing or invalid Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: youngmrz <[email protected]>
When an invalid PR number is passed to --pr, the script now exits with code 1 instead of silently continuing with 0 passed/0 failed. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: youngmrz <[email protected]>
Added documentation to log(), success(), error(), info(), and warn() functions to meet 80% docstring coverage threshold. Co-Authored-By: Claude Opus 4.5 <[email protected]>
youngmrz
force-pushed
the
util/windows-test-script
branch
from
e10342d to
c2fc9c4
Compare
Address Sentry bot feedback that testPR4_TokenHotReload fails hard when the CONFIG_RELOAD and reloadConfig code doesn't exist (because the PR hasn't been merged yet). This change aligns the test behavior with PR #1 and PR #2 tests which gracefully skip when their respective code is absent. Changes: - Track found/missing counts instead of a single allPassed boolean - Use info() and warn() instead of error() for "not found" messages - Return { passed: true, skipped: true } when no hot-reload code is found - Return { passed: false } only for partial implementations (some code present but not all) Co-Authored-By: Claude Opus 4.5 <[email protected]>
3 participants
Add a script to verify Windows-specific PRs are working correctly:
Usage: node scripts/test-windows-fixes.js [--all] [--pr N]
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Related Issue
Closes #
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemChecklist
developbranchCI/Testing Requirements
Screenshots
Feature Toggle
use_feature_nameBreaking Changes
Breaking: Yes / No
Details:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.